-
-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: pattern match of topLevelImportPaths was unanchored #368
fix: pattern match of topLevelImportPaths was unanchored #368
Conversation
This seems to have been a mistake in styled-components#354
It's unclear to me that this causes breakage, the option is undocumented and normally the empty array, so how that's causing breakage is confusing to me. Anyways, anchoring this regex should not be forced here in my opinion. And the built-in matches are bounded. The test change looks good though |
@rockwotj It's rarely empty because the most common way to use the plugin is through the macro |
The point is to preserve some semblance of backward compat. Bare strings will probably work as expected so long as they're anchored. For advanced cases, you can easily prepend or append |
That's fair, note the opinion name changed so that macro code doesn't work either. EDIT: ah nevermind it's not renamed. |
Anyways since this is also a "breaking" change, should we just switch to picomatch? |
I agree it's a breaking change at this point. I really liked your suggestion to use picomatch earlier—I think it could have prevented the original breaking change, because it's extremely unlikely that any ordinary module strings would contain picomatch globbing chars. I don't have a strong feeling about using picomatch now, except to avoid this getting stuck in a cycle of well-meaning "should we just" alternatives. 😬 Anchoring is near at hand, but if you want to rework it to use picomatch, I'd support that.
Unfortunately, I don't think this is the only problem involved. Between styled-components, this plugin, and the macro, there are a lot of issues open at the moment, with various claims that pinning one thing or the other solves it. Unfortunately, working on this amounts to yak shaving for most of us, myself included. I'm trying to gradually work through them, though. |
Ah so I tried and picomatch was not able to support "going up" an arbitrary number of directories, which is often the usecase for relative imports ( FWIW I tried out the tests in the styled-components repo for the macro and they pass with the latest version, so I'm not sure what the difference is. I did send a PR to fix this in the macro, but if we merge this we'll probably want to modify that PR to remove the bounds. |
Not advocating for picomatch, but this works:
See https://github.com/micromatch/picomatch#extglobs If we still want to consider alternatives, what about your other suggestion to use objects? It could be: {
"topLevelImportPaths": [
{"match": "/my-local-styled-components$"}
]
} Existing strings would be used as literals, so you'd opt into the regex syntax explicitly with the object format. Frankly, I would probably prefer that over the approach in this PR, but I'd take either one! 😂 |
Ah good find with the extended glob patterns. I've opened a PR to use picomatch: #369 |
Closing in favor of #369 |
This seems to have been a mistake in #354
It might fix styled-components/styled-components#3635 but that's somewhat involved to test while this is unreleased, so it might be easier to make this release and find out.